Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(userspace): change falco engine design to properly support multiple sources #2017

Merged
merged 10 commits into from
May 25, 2022

Conversation

jasondellaluce
Copy link
Contributor

@jasondellaluce jasondellaluce commented May 24, 2022

What type of PR is this?

/kind bug

/kind cleanup

/kind design

/kind feature

Any specific area of the project related to this PR?

This PR refactors and cleans up the design of the Falco Engine classes. Since introducing the notion of "multiple sources" due to the plugin system, the state management and the responsibilities of the internal components of the engine became ambiguous. The goal of these changes is to polish the design of the Falco Engine internals without introducing breaking changes in its interface, making it more extendible, and less error-prone. Overall, the changes involve:

  • Removing cyclic dependencies and forward declarations inside the engine
  • Defining documented structs for falco rules and sources, each in its standalone header
  • Separating state management responsibilities in the engine
  • Adding a complete_rule_loading() method in falco_engine to make the engine explicitly know when all rules have been loaded and enabled/disabled by Falco. So far, this knowledge has always been implicit.
  • Introducing an interface for filter rulesets named filter_ruleset. This allows injecting multiple ruleset implementations inside the engine, each with different indexing/optimization strategies. This will serve us in the future to have plugin-specific indexing strategies for rule matching because the current evttype-based is totally ineffective. The default ruleset implementation is still the evttype-indexing one, which is now a standalone class named evttype_index_ruleset with explicit responsibilities.
  • Documenting all the newly-introduced classed and some of the legacy ones too

/area engine

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

The PR is big in LOC due to some parts of code being moved, however ~200 of those are related to unit test being ported to the new method definitions.

Does this PR introduce a user-facing change?:

NONE

@leogr
Copy link
Member

leogr commented May 24, 2022

/milestone 0.32.0

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can only say thank you for the effort you have spent in improving those parts of the codebase.
🙏

I reserve some time to take a second look, but it SGTM. Just left a suggestion.

userspace/engine/ruleset.h Outdated Show resolved Hide resolved
leogr
leogr previously approved these changes May 24, 2022
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I give it several tries and everything works as expected.

Thank you!

/approve

@poiana
Copy link
Contributor

poiana commented May 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jasondellaluce, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [jasondellaluce,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana
Copy link
Contributor

poiana commented May 24, 2022

LGTM label has been added.

Git tree hash: 601628490bfc34691c108504061066ee49c54ef5

jasondellaluce and others added 8 commits May 24, 2022 17:09
…r factory

This interface will allow us to use different ruleset implementations inside the same engine.
The goal is to define API boundaries that will allow swapping the current evttype-index
ruleset implementation more easily. Key benefits include: smaller component with less responsibilities,
easier substituibility, more testable design, opportunity to adopt different index strategies
depending on the ruleset implementation.

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…new filter_ruleset interface

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…terface and have one ruleset for each source

This also fixes a couple of bugs. With the current implementation, the multi-ruleset feature is broken with multiple sources.

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
…set interface

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
The filter_ruleset interface its implementation evt_type_index_ruleset
have been modified as follows:
- Only keep track of ruleset ids and not names. The falco engine will take
care of mapping easy-to-remember ruleset names to ruleset ids.
To emphasize this, use ruleset_id everywhere and not ruleset.
Also, make it non-optional.
- Have explicit separate functions to enable/disable rules, instead of a single enable() method combined with a boolean flag.
This does *not* change the falco_engine interface, which has
similar methods, to avoid breaking API changes.

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Co-authored-by: Mark Stemm <mark.stemm@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
This updates the engine to comply and work properly with the newly-introduced
interface design.

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
jasondellaluce and others added 2 commits May 24, 2022 17:09
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
Co-authored-by: Leonardo Grasso <me@leonardograsso.com>
@jasondellaluce jasondellaluce force-pushed the refactor/ruleset-factory branch from 67bbee2 to 2de6182 Compare May 24, 2022 17:42
@poiana poiana removed the lgtm label May 24, 2022
@poiana poiana requested a review from leogr May 24, 2022 17:42
Copy link
Contributor

@mstemm mstemm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making this valuable set of changes!

@poiana
Copy link
Contributor

poiana commented May 24, 2022

LGTM label has been added.

Git tree hash: f6d95256d7dd68b8a0935af9317a405f9978fc90

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thank you!

yay! 🥳

@poiana poiana merged commit 13d70b6 into master May 25, 2022
@poiana poiana deleted the refactor/ruleset-factory branch May 25, 2022 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants